Fix random_clifford
#13606
Fix random_clifford
#13606
Conversation
|
One or more of the following people are relevant to this code:
|
alexanderivrii
added
the
Changelog: Bugfix
There was a problem hiding this comment.
Thanks @alexanderivrii for fixing this bug!
The tests for 2-qubit Cliffords look good to me.
I wonder if we can also test larger Cliffrods, say with 3 and 4 qubits. We can't check that we generate the entire group (it's just too large), but perhaps we can check that they at least produce distinct CX counts (according to their popularity)?
See this table:
<style> </style>| Num of CNOTS | 1 qubit | 2 qubits | 3 qubits | 4 qubits |
|---|---|---|---|---|
| Paulis | 4 | 16 | 64 | 256 |
| 0 | 6 | 36=6^2 | 216=6^3 | 1296=6^4 |
| 1 | - | 324=9*36 | 27*216 | 54*1296 |
| 2 | - | 324=9*36 | 432*216 | 1917*1296 |
| 3 | - | 36 | 2784*216 | 47904*1296 |
| 4 | - | - | 3042*216 | 849438*1296 |
| 5 | - | - | 432*216 | 8192583*1296 |
| 6 | - | - | 2*216 | 22623581*1296 |
| 7 | - | - | - | 4809996*1296 |
| 8 | - | - | - | 31320*1296 |
| 9 | - | - | - | 6*1296 |
| Total | 24=4x6 | 11,520= 16x36x20 | 6720x216x64= 92,897,280 | 36,556,800x1296x256 =12,128,668,876,800 |
Pull Request Test Coverage Report for Build 12631085532Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
The failing test seems unrelated to this PR: |
|
@ShellyGarion, based on your suggestion, I have added a test (see 97b3cc9) that checks that we also get all possible 2-qubit depths for random 3-qubit cliffords, also note that in this case for decomposing Cliffords we use the optimal Bravyj-Maslov algorithm, so the CX-counts are exact. The test considers 10,000 random Cliffords and is still reasonably fast, taking 1s on my laptop. However, for 4-qubit Cliffords, the chosen decomposition method is only heuristic and we don't necessarily get the exact counts. In addition, it requires significantly more tests (and more time) to hit the low-popularity counts. I have locally verified that we seem to get the approximately expected counts, but for the reasons above I don't think we should add an explicit test for that. |
|
It looks like this missed Qiskit 1.3.2. Should it still be backported to 1.3? |
|
Yeah, looks like it didn't get tagged for backport despite the milestone. I'll shift to hopefully 1.3.3. |
jakelishman
added
the
stable backport potential
* fix and tests * Using a fixed rather than random clifford in one of the tests * reno * restoring previous docstring that was changed accidentally * updating in-code comment * also checking decompositions for random 3q cliffords (cherry picked from commit 0d4ee23)
* fix and tests * Using a fixed rather than random clifford in one of the tests * reno * restoring previous docstring that was changed accidentally * updating in-code comment * also checking decompositions for random 3q cliffords (cherry picked from commit 0d4ee23) Co-authored-by: Alexander Ivrii <[email protected]>
Summary
Fixes #13590.
Details and comments
The bug was introduced when porting
random_cliffordto Rust in #12695.I have added tests that ensure that for 2-qubits Cliffords,
random_cliffordproduces Cliffords with all possible symplectic matrices (there should be 720) and all possible CX counts (should be {0, 1, 2, 3}). @ShellyGarion, can you think of other tests that we may wish to add?